C++: Range analysis performance fix #20816
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
|
|
||
| float debugNrOfBounds(Expr e) { | ||
| e = getRelevantLocatable() and |
Check warning
Code scanning / CodeQL
Missing QLDoc for parameter Warning
| * should be empty, so this predicate is useful to debug non-functional cases. | ||
| */ | ||
| int nonFunctionalNrOfBounds(Expr e) { | ||
| strictcount(BoundsEstimate::nrOfBoundsExpr(e)) > 1 and |
Check warning
Code scanning / CodeQL
Missing QLDoc for parameter Warning
| * Finds any expressions for which `nrOfBounds` is not functional. The result | ||
| * should be empty, so this predicate is useful to debug non-functional cases. | ||
| */ | ||
| private predicate nonFunctionalNrOfBounds(Expr e) { |
Check warning
Code scanning / CodeQL
Missing QLDoc for parameter Warning test
paldepind
force-pushed
the
cpp/range-analysis-perf-fix
branch
from
88b9fc4 to
ffe9839
Compare
paldepind
force-pushed
the
cpp/range-analysis-perf-fix
branch
from
ffe9839 to
1dd78e2
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR improves the performance of C++ range analysis by fixing a bug that caused nrOfBoundsNEPhi and nrOfBoundsUnsupportedGuardPhi to be non-functional. The fix uses max instead of exists to ensure only a single access is considered when multiple accesses exist for a given variable and definition. This prevents combinatorial explosion during range analysis and fixes previously observed timeouts.
Key changes:
- Introduces bound estimation module to predict when widening is needed
- Adds helper functions for consistent widening application
- Fixes non-functional predicates using
maxaggregate - Adds comprehensive test cases for performance edge cases
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cpp/ql/lib/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll | Core implementation with bounds estimation module, widening helpers, and performance optimizations |
| cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/test.c | New test functions for repeated if statements, NE phi nodes, and nested guards |
| cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/nrOfBounds.ql | New test query to verify functionality of bounds estimation |
| cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/*.expected | Updated expected test outputs reflecting the fix |
| cpp/ql/lib/change-notes/2025-11-11-range-analysis-performance.md | Change note documenting the performance fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cpp/ql/lib/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
jketema
left a comment
jketema
left a comment
There was a problem hiding this comment.
LGTM. Just for completeness would you mind running DCA on this?
|
Yes, of course. |
2 participants
This re-applies the reverted #20645 and fixes a bug that caused
nrOfBoundsNEPhito be non-functional. For a givenvanddefthe propertyisNEPhi(v, def, access, _)might hold for manyaccesses. Hence we can't useexistsforaccess. Usingmaxinstead ensures that we only propagate bounds for a singleaccess.nrOfBoundsUnsupportedGuardPhisuffered from the same problem. I haven't seen any issues arise in practice from that, but I gave it the same treatment for good measure.A QA run has showed that this fixes the previously seen timeouts. We now instead gain 7 stable progressions on BMN and 14 stable progressions on traced extraction, at the cost of slowdowns in the 0.8 - 1.8x range on 3 projects.